Skip to content

[Draft] Modified OnScreenControl to support a customisable update mode #1792

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 7 commits into
base: develop
Choose a base branch
from
Draft

[Draft] Modified OnScreenControl to support a customisable update mode #1792

wants to merge 7 commits into from

Conversation

@ekcoh
Copy link
Collaborator

@ekcoh ekcoh commented Nov 5, 2023

Description

Added a new "update mode" property on OnScreenControl.

Updated the OnScreenControl example in /samples.

Changes made

Feature addition to OnScreenControl:

  • Added new enumeration type to public API OnScreenControlUpdateMode which allows the user to specify whether the OnScreenControl should output its input data by queuing input events to the system (OnScreenControlUpdateMode.Queue, default) or whether the OnScreenControl should output its input data by changing the underlying control state via InputState.Change(...) directly (OnScreenControlUpdateMode.StateChange, new optional mode).

Updated the OnScreenControl example:

  • Added OnScreenControlSample.inputactions defining actions corresponding to OnScreenControls used in example.
  • Added OnScreenControlSample.cs hooking up performed handlers to log when action events fire.
  • Modified A, B to correspond to Gamepad controls instead of Keyboard to not confuse users.
  • Added Mode 1 and Mode 2 buttons mapping to Keyboard controls instead and allowing these to be used to switch update mode interactively when running the sample.
  • Added inline documentation explaining how the sample is rigged up.

Removed previous FIXME relating to this.

Notes

Note that PR still contains debug logging in OnScreenStick and OnScreenButton.

Note that updating via InputState.Change(...) avoids delaying generated until the next frame and effectively reduces latency of OnScreenControl by one frame or equivalently 1 / FrameRate seconds. However, this cannot be the default value since it would break backwards compability. However, OnScreenControlUpdateMode.StateChange have been highlighted as the recommended setting.

This PR needs significant testing and QA before considered to be merged since potential side-effects or detection of improper control layouts for this update pattern are currently not clear.

Checklist

Before review:

  • Changelog entry added.
    • Explains the change in Changed, Fixed, Added sections.
    • For API change contains an example snippet and/or migration example.
    • FogBugz ticket attached, example ([case %number%](https://issuetracker.unity3d.com/issues/...)).
    • FogBugz is marked as "Resolved" with next release version correctly set.
  • Tests added/changed, if applicable.
    • Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
    • Performance tests.
    • Integration tests.
  • Docs for new/changed API's.
    • Xmldoc cross references are set correctly.
    • Added explanation how the API works.
    • Usage code examples added.
    • The manual is updated, if needed.

During merge:

  • Commit message for squash-merge is prefixed with one of the list:
    • NEW: ___.
    • FIX: ___.
    • DOCS: ___.
    • CHANGE: ___.
    • RELEASE: 1.1.0-preview.3.

ritamerkl and others added 4 commits November 1, 2023 17:21
@ritamerkl
fix for neutron CI warning (#1787)
4c61a30
@jamesmcgill
FIX: UITK Listen button not working with Any control types (#1788)
fa5f304 
* FIX: UITK Listen button not working with Any control types

* Remove jira links for ISX project issues from changelog
@ekcoh
NEW: Added update mode enum to OnScreenControl to allow changing betw…
e4560c2 
…een queue-based and state-change based updates.
@ekcoh
Evolved OnScreen sample and OnScreenControls
9f09313
@ekcoh ekcoh added the work in progress Indicates that the PR is work in progress and any review efforts can be post-poned. label Nov 5, 2023
@ekcoh ekcoh self-assigned this Nov 6, 2023
jamesmcgill
jamesmcgill reviewed Nov 6, 2023
View reviewed changes
Copy link
Collaborator

@jamesmcgill jamesmcgill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Draft looks good 👍

Unless there is a good reason for keeping the old behaviour I would recommend just making the new behaviour the only behaviour.
Otherwise we have more public API surface and more scenarios/code paths to maintain.
The old behaviour just seems completely wrong to me and I would class it as a bug.

@ekcoh
Copy link
Collaborator Author

ekcoh commented Nov 6, 2023

Agree @jamesmcgill that I think it should replace previous behaviour. However I think it could regress existing implementations relying on this, but frankly it should be seen as a bug since previous implementation is not providing what you want.
Will revisit this again when it may be prioritised and review API contracts in light of making this the default behavior.
For the sake of the example I also believe there would be much to gain to make it realistic from a UI point of view, at least from a physically scaled and alignment perspective since the current UI representation isn't usable for cross-device usage and doesn't even have a usable layout to begin with. The sample improvement could however be separated from the functional change, as long as the functional change comes with test coverage which is currently missing.

@ekcoh
Copy link
Collaborator Author

ekcoh commented Oct 21, 2025

If work is resumed on this PR it needs to be updated with latest changes from develop to get a critical CI fix to not cause CI issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jamesmcgill jamesmcgill jamesmcgill left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@ekcoh ekcoh

Labels

work in progress Indicates that the PR is work in progress and any review efforts can be post-poned.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ekcoh @jamesmcgill @ritamerkl